Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

release 1.2.0-rc.1 #4221

Merged
merged 5 commits into from
Apr 3, 2024
Merged

release 1.2.0-rc.1 #4221

merged 5 commits into from
Apr 3, 2024

Conversation

cyphar
Copy link
Member

@cyphar cyphar commented Mar 14, 2024

This is the first release candidate for the 1.2.0 branch of runc. It includes
all patches and bugfixes included in runc 1.1 patch releases (up to and
including 1.1.12). A fair few new features have been added, and some changes
have been made which may affect users. Please help us thoroughly test this
release before we release 1.2.0.

1.2.0-rc.1 - 2024-04-01

There's a frood who really knows where his towel is.

runc now requires a minimum of Go 1.20 to compile.

NOTE: runc currently will not work properly when compiled with Go 1.22 or
newer. This is due to some unfortunate glibc behaviour that Go 1.22
exacerbates in a way that results in containers not being able to start on
some systems. See this issue for more information.

Breaking

  • Several aspects of how mount options work has been adjusted in a way that
    could theoretically break users that have very strange mount option strings.
    This was necessary to fix glaring issues in how mount options were being
    treated. The key changes are:

    • Mount options on bind-mounts that clear a mount flag are now always
      applied. Previously, if a user requested a bind-mount with only clearing
      options (such as rw,exec,dev) the options would be ignored and the
      original bind-mount options would be set. Unfortunately this also means
      that container configurations which specified only clearing mount options
      will now actually get what they asked for, which could break existing
      containers (though it seems unlikely that a user who requested a specific
      mount option would consider it "broken" to get the mount options they
      asked foruser who requested a specific mount option would consider it
      "broken" to get the mount options they asked for). This also allows us to
      silently add locked mount flags the user did not explicitly request to be
      cleared
      in rootless mode, allowing for easier use of bind-mounts for
      rootless containers. (rootfs: remove --no-mount-fallback and finally fix MS_REMOUNT #3967)

    • Container configurations using bind-mounts with superblock mount flags
      (i.e. filesystem-specific mount flags, referred to as "data" in
      mount(2), as opposed to VFS generic mount flags like MS_NODEV) will
      now return an error. This is because superblock mount flags will also
      affect the host mount (as the superblock is shared when bind-mounting),
      which is obviously not acceptable. Previously, these flags were silently
      ignored so this change simply tells users that runc cannot fulfil their
      request rather than just ignoring it. (configs: validate: add validation for bind-mount fsflags #3990)

    If any of these changes cause problems in real-world workloads, please open
    an issue
    so we
    can adjust the behaviour to avoid compatibility issues.

Added

Deprecated

Changed

Fixed

Removed

@cyphar cyphar mentioned this pull request Mar 14, 2024
@lifubang
Copy link
Member

I think #4210 is a bug introduced by #3749 which is included in this release, how about waiting this issue fixed?
But I don't know we should only fix this special issue, or cherry-pick from c9ba576 to fix all potential issues about numeric pointer.

@lifubang
Copy link
Member

Maybe we should include #4222 .

Copy link
Member

@rata rata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some simple questions, but this LGTM after those clarifications.

CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@rata
Copy link
Member

rata commented Mar 14, 2024

@lifubang

how about waiting this issue fixed?

IMHO those are minor things, with small PRs. I'm fine with either merging or just releasing the rc now and adding those small things later. There is definitely lot of things merged already and getting this out would be very useful in any case (with or without those small fixes)

@lifubang
Copy link
Member

IMHO those are minor things, with small PRs.

I have updated #4210 , I think this is not a small bug. Maybe I wrote this issue not clearly before.
I think before we draft a release, if we have realized that there is a small bug, we should fix it.

Copy link
Member

@rata rata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@lifubang lifubang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I was debugging #4211 , there was another bug: #4225 .
I think I can't solve it in a short time.
So let's record this bug and release 1.2.0-rc.1.

@cyphar
Copy link
Member Author

cyphar commented Mar 25, 2024

@kolyshkin WDYT? Should we wait for #4227 or release this first and make sure we get #4227 for 1.2.0?

@kolyshkin
Copy link
Contributor

Should we wait for #4227 or release this first and make sure we get #4227 for 1.2.0?

I found out that it requires some major rework, so I can't really fix it right now.

Let'd do rc1 soon. I will review this PR tomorrow.

@kolyshkin
Copy link
Contributor

LGTM overall.

I'm concerned about Go 1.22 vs glibc vs nsenter incompatibility issue (described in golang/go#65625).

I think we should at least add a note to Changelog about it, and at most don't allow runc to be compiled with Go 1.22. Something like this should suffice:

$ cat <<< EOF > ./libcontainer/nsenter/nsenter_go122.go 
//go:build go1.22

package nsenter

import "runc does not work with Go 1.22"
EOF

Result:

go1.22.0 build -trimpath "-buildmode=pie"  -tags "seccomp urfave_cli_no_docs " -ldflags "-X main.gitCommit=v1.1.0-987-g59078892 -X main.version=1.1.0+dev " -o runc .
init.go:7:2: /home/kir/git/runc/libcontainer/nsenter/nsenter_go122.go:5:8: invalid import path: runc does not work with Go 1.22
make: *** [Makefile:71: runc-bin] Error 1

CHANGELOG.md Outdated Show resolved Hide resolved
@SuperQ
Copy link
Contributor

SuperQ commented Mar 28, 2024

@kolyshkin Looking over the Go issue. It seems like the issue is not Go, but glibc and nsenter. And then, only specific versions of those. It seems like they improved the error handling in Go, but I have not checked the 1.22.1 changelog to see if those patches are there.

Hard dropping Go 1.22 like that is, IMO, not an acceptable way to deal with this.

Kubernetes, which directly uses this project, is already building with 1.22 as a minimum required version.

@cyphar
Copy link
Member Author

cyphar commented Mar 28, 2024

@SuperQ The issue with Go 1.22 is a little more complicated than that AFAIK.

Yes, the core issue is that arguably nsenter needs to be reworked to do another re-exec after it sets up the namespaces (yay, yet another thing to make Go container runtimes slower!) but the reason that Go 1.22 will aggravate this issue is because they now explicitly detect the broken pthread state case and always panic (this patch wasn't merged AFAICS but it must've been carried -- I haven't looked into it much after I saw this patch).

Given the severity of the issue, I will see if I can get a fix for this before 1.2.0. But we shouldn't block 1.2.0-rc.1 for this issue IMHO.

(I suspect dropping CLONE_PARENT would be a workable "hacky" fix, but doing so will make zombie reaping more annoying and is a temporary fix since future glibc changes could cause issues.)

Kubernetes, which directly uses this project, is already building with 1.22 as a minimum required version.

Adding it to libcontainer/nsenter means that users which do not import nsenter will not have build issues. Kubernetes does not import nsenter so this will not impact Kubernetes. The only downstreams it will impact are those that use nsenter -- which is precisely the users that need to avoid building with Go 1.22 for the time being.

@cyphar

This comment was marked as resolved.

Just to make sure we don't forget to fully explain these when we do
-rc1.

Signed-off-by: Aleksa Sarai <[email protected]>
Signed-off-by: Rodrigo Campos <[email protected]>
[ cyphar: restructuring and removal of outdated or incorrect info ]
Signed-off-by: Aleksa Sarai <[email protected]>
@cyphar cyphar force-pushed the release-1.2.0-rc1 branch 2 times, most recently from f63426e to 0e50df2 Compare April 2, 2024 04:48
@cyphar
Copy link
Member Author

cyphar commented Apr 2, 2024

@kolyshkin Now that we refuse to build with Go 1.22, does this look okay to release to you?

Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉

CHANGELOG.md Outdated Show resolved Hide resolved
@cyphar cyphar merged commit 0680e2b into opencontainers:main Apr 3, 2024
45 checks passed
@cyphar cyphar deleted the release-1.2.0-rc1 branch April 3, 2024 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants